Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define program-wide Rust flags in .bazelrc #454

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Define program-wide Rust flags in .bazelrc #454

merged 1 commit into from
Mar 23, 2023

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Mar 16, 2023

The flags defined so far in rust-deps/BUILD.bazel are not applied to dependent targets, so manually define global Rust flags to enable -C panic=abort and improve code size. Linux opt binaries are ~280KB smaller as a result.
I also considered a number of other configurations and identified some options that might be good for release builds, the comments describe how these might be used in a future release configuration.

Upstream PR has been added.

@fhanau fhanau requested a review from mikea March 16, 2023 21:15
@fhanau fhanau force-pushed the felix/rust-flags branch 2 times, most recently from 4360811 to e778241 Compare March 18, 2023 14:30
@fhanau fhanau changed the title Define Rust flags in .bazelrc Define program-wide Rust flags in .bazelrc Mar 18, 2023
@fhanau fhanau marked this pull request as ready for review March 18, 2023 14:33
@kentonv
Copy link
Member

kentonv commented Mar 20, 2023

Do we want panics to be aborts? This differs significantly from our philosophy on the C++ side, where we use exceptions like panics, but we very much want to be able to catch them, so that we can fail out just one request instead of crashing the server.

@fhanau
Copy link
Collaborator Author

fhanau commented Mar 21, 2023

Do we want panics to be aborts? This differs significantly from our philosophy on the C++ side, where we use exceptions like panics, but we very much want to be able to catch them, so that we can fail out just one request instead of crashing the server.

panic=abort is what we are currently using based on rust-deps/BUILD.bazel (although that's not applied globally), I assumed that it was best to keep it

For fastdbg panic=unwind should be preferable either way, I'm also open to changing it in the general case

@kentonv
Copy link
Member

kentonv commented Mar 22, 2023

Ah OK, makes sense.

(I'm not competent to review this diff, will leave it to Mike.)

@fhanau fhanau merged commit fd2e7cd into main Mar 23, 2023
@fhanau fhanau deleted the felix/rust-flags branch April 1, 2023 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants